Skip to content

Conversation

zachs18
Copy link
Contributor

@zachs18 zachs18 commented May 13, 2024

Split out from #119761

Add fn into_raw_with_allocator for Rc/rc::Weak1/Arc/sync::Weak.

  • Pairs with from_raw_in (which already exists on all 4 types).
  • Name matches Box::into_raw_with_allocator.
  • Associated fns on Rc/Arc, methods on Weaks.
Future PR/ACP

As a follow-on to this PR, I plan to make a PR/ACP later to move into_raw(_parts) from Container<_, A: Allocator> to only Container<_, Global> (where Container = Vec/Box/Rc/rc::Weak/Arc/sync::Weak) so that users of non-Global allocators have to explicitly handle the allocator when using into_raw-like APIs.

The current behaviors of stdlib containers are inconsistent with respect to what happens to the allocator when into_raw is called (which does not return the allocator)

Type into_raw currently callable with behavior of into_raw
Box any allocator allocator is dropped
Vec any allocator allocator is forgotten
Arc/Rc/Weak any allocator allocator is forgotten(Arc) (sync::Weak) (Rc) (rc::Weak)

In my opinion, neither implicitly dropping nor implicitly forgetting the allocator is ideal; dropping it could immediately invalidate the returned pointer, and forgetting it could unintentionally leak memory. My (to-be) proposed solution is to just forbid calling into_raw(_parts) on containers with non-Global allocators, and require calling into_raw_with_allocator(/Vec::into_raw_parts_with_alloc)

Footnotes

  1. Technically, rc::Weak::into_raw_with_allocator is not newly added, as it was modified and renamed from rc::Weak::into_raw_and_alloc.

@rustbot
Copy link
Collaborator

rustbot commented May 13, 2024

r? @Mark-Simulacrum

rustbot has assigned @Mark-Simulacrum.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels May 13, 2024
@zachs18

This comment was marked as resolved.

@rustbot rustbot added the A-allocators Area: Custom and system allocators label May 13, 2024
@zachs18 zachs18 marked this pull request as draft May 13, 2024 22:59
@rust-log-analyzer

This comment has been minimized.

@zachs18

This comment was marked as outdated.

@zachs18

This comment was marked as outdated.

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 14, 2024
... since fn allocator doesn't exist yet.
@zachs18
Copy link
Contributor Author

zachs18 commented May 17, 2024

Replaced uses of fn allocator (which doesn't exist yet) with directly accessing the alloc field. No longer blocked on #124980.

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 17, 2024
@zachs18 zachs18 marked this pull request as ready for review May 17, 2024 03:12
@Mark-Simulacrum
Copy link
Member

@bors r+

@bors
Copy link
Collaborator

bors commented May 19, 2024

📌 Commit c895f6e has been approved by Mark-Simulacrum

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 19, 2024
bors added a commit to rust-lang-ci/rust that referenced this pull request May 20, 2024
…iaskrgr

Rollup of 5 pull requests

Successful merges:

 - rust-lang#125034 (Weekly `cargo update`)
 - rust-lang#125093 (Add `fn into_raw_with_allocator` to Rc/Arc/Weak.)
 - rust-lang#125282 (Never type unsafe lint improvements)
 - rust-lang#125301 (fix suggestion in E0373 for !Unpin coroutines)
 - rust-lang#125302 (defrost `RUST_MIN_STACK=ice rustc hello.rs`)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 7389416 into rust-lang:master May 20, 2024
@rustbot rustbot added this to the 1.80.0 milestone May 20, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request May 20, 2024
Rollup merge of rust-lang#125093 - zachs18:rc-into-raw-with-allocator-only, r=Mark-Simulacrum

Add `fn into_raw_with_allocator` to Rc/Arc/Weak.

Split out from rust-lang#119761

Add `fn into_raw_with_allocator` for `Rc`/`rc::Weak`[^1]/`Arc`/`sync::Weak`.
* Pairs with `from_raw_in` (which already exists on all 4 types).
* Name matches `Box::into_raw_with_allocator`.
* Associated fns on `Rc`/`Arc`, methods on `Weak`s.

<details> <summary>Future PR/ACP</summary>

As a follow-on to this PR, I plan to make a PR/ACP later to move `into_raw(_parts)` from `Container<_, A: Allocator>` to only `Container<_, Global>` (where `Container` = `Vec`/`Box`/`Rc`/`rc::Weak`/`Arc`/`sync::Weak`) so that users of non-`Global` allocators have to explicitly handle the allocator when using `into_raw`-like APIs.

The current behaviors of stdlib containers are inconsistent with respect to what happens to the allocator when `into_raw` is called (which does not return the allocator)

| Type | `into_raw` currently callable with | behavior of `into_raw`|
| --- | --- | --- |
| `Box` | any allocator | allocator is [dropped](https://doc.rust-lang.org/nightly/src/alloc/boxed.rs.html#1060) |
| `Vec` | any allocator | allocator is [forgotten](https://doc.rust-lang.org/nightly/src/alloc/vec/mod.rs.html#884) |
| `Arc`/`Rc`/`Weak` | any allocator | allocator is [forgotten](https://doc.rust-lang.org/src/alloc/sync.rs.html#1487)(Arc) [(sync::Weak)](https://doc.rust-lang.org/src/alloc/sync.rs.html#2726) [(Rc)](https://doc.rust-lang.org/src/alloc/rc.rs.html#1352) [(rc::Weak)](https://doc.rust-lang.org/src/alloc/rc.rs.html#2993) |

In my opinion, neither implicitly dropping nor implicitly forgetting the allocator is ideal; dropping it could immediately invalidate the returned pointer, and forgetting it could unintentionally leak memory. My (to-be) proposed solution is to just forbid calling `into_raw(_parts)` on containers with non-`Global` allocators, and require calling `into_raw_with_allocator`(/`Vec::into_raw_parts_with_alloc`)

</details>

[^1]:  Technically, `rc::Weak::into_raw_with_allocator` is not newly added, as it was modified and renamed from `rc::Weak::into_raw_and_alloc`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-allocators Area: Custom and system allocators S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants